Resolved TODO in compare_cavity_external.py#267
Conversation
Implemented logic to interpolate external result onto the cfd_python grid using `scipy.interpolate.RectBivariateSpline`. Addressed grid size mismatch and updated centerlines after interpolation. Also added .venv to .gitignore. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the validation script for cavity flow comparisons by introducing robust handling for discrepancies in grid sizes between computational fluid dynamics (CFD) results and external reference solutions. By automatically interpolating external data onto the CFD grid, the change ensures consistent and accurate comparisons, thereby improving the reliability of the validation process. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThe changes add virtual environment directory patterns to the ignore file and enhance grid comparison logic in a cavity validation script by implementing interpolation when external and internal CFD grids have different dimensions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a TODO by implementing interpolation to handle grid mismatches in compare_cavity_external.py, which is a good improvement for the validation script. The change to .gitignore is also appropriate. However, the current implementation introduces a bug in plot_comparison that will cause a crash if a grid mismatch occurs and interpolation is performed. I've provided a review comment with a suggested fix to address this issue by ensuring the plotting grid is also updated after interpolation.
| # Update centerlines | ||
| mid_x_idx = len(x_cfd) // 2 | ||
| mid_y_idx = len(y_cfd) // 2 | ||
| ext_sol["u_centerline"] = ext_sol["u"][:, mid_x_idx] | ||
| ext_sol["v_centerline"] = ext_sol["v"][mid_y_idx, :] |
There was a problem hiding this comment.
While the interpolation logic correctly resamples the external solution fields (u, v, p) onto the CFD grid, the plotting function plot_comparison will fail with a dimension mismatch error. This is because plot_comparison uses the grid coordinates from the ext_solver object (ext_solver.X, ext_solver.Y, ext_solver.x, ext_solver.y), which are not updated after interpolation.
To fix this, you should also update these grid attributes on the ext_solver object to match the CFD grid on which the external solution has been interpolated. This will ensure that subsequent plotting calls use consistent data and grid dimensions.
| # Update centerlines | |
| mid_x_idx = len(x_cfd) // 2 | |
| mid_y_idx = len(y_cfd) // 2 | |
| ext_sol["u_centerline"] = ext_sol["u"][:, mid_x_idx] | |
| ext_sol["v_centerline"] = ext_sol["v"][mid_y_idx, :] | |
| # Update centerlines | |
| mid_x_idx = len(x_cfd) // 2 | |
| mid_y_idx = len(y_cfd) // 2 | |
| ext_sol["u_centerline"] = ext_sol["u"][:, mid_x_idx] | |
| ext_sol["v_centerline"] = ext_sol["v"][mid_y_idx, :] | |
| # Update grid on external solver object for consistent plotting | |
| ext_solver.x = x_cfd | |
| ext_solver.y = y_cfd | |
| ext_solver.X, ext_solver.Y = np.meshgrid(x_cfd, y_cfd) |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on d0aa6c2..9b6fbc6
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (2)
• .gitignore
• validation/compare_cavity_external.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
validation/compare_cavity_external.py (2)
148-154: Consider recomputing vortex center after interpolation.After interpolation,
ext_sol["vortex_center"]still holds the value computed from the original external grid. For consistency with the interpolated data, you could recompute it from the interpolated pressure field, similar to how it's computed forcfd_python_result:if ext_sol.get("_interpolated"): ext_vortex_idx = np.unravel_index(np.argmin(ext_sol["p"]), ext_sol["p"].shape) external_vortex = (x_cfd[ext_vortex_idx[1]], y_cfd[ext_vortex_idx[0]]) else: external_vortex = ext_sol["vortex_center"]This is a minor concern since the physical vortex location should be similar, but worth noting for accuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/compare_cavity_external.py` around lines 148 - 154, The external vortex center is left as the original-grid value (ext_sol["vortex_center"]) even when ext_sol was interpolated; detect interpolation via ext_sol.get("_interpolated") and if True recompute the vortex from the interpolated pressure field by finding ext_vortex_idx = np.unravel_index(np.argmin(ext_sol["p"]), ext_sol["p"].shape) and setting external_vortex = (x_cfd[ext_vortex_idx[1]], y_cfd[ext_vortex_idx[0]]); otherwise keep ext_sol["vortex_center"]. Update the comparison block that currently uses cfd_python_result, cfd_python_vortex and external_vortex to use this recomputed value when appropriate.
115-121: Consider adding input validation forRectBivariateSpline.
RectBivariateSplinerequiresxandyarrays to be strictly monotonically increasing. If the external solver could produce non-monotonic coordinates, this would raise an error. For robustness, you might add a quick assertion:assert np.all(np.diff(x_ext) > 0) and np.all(np.diff(y_ext) > 0), \ "External grid coordinates must be monotonically increasing for interpolation"This is likely fine for uniform cavity grids but would help catch unexpected inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/compare_cavity_external.py` around lines 115 - 121, Add input validation before creating the RectBivariateSpline objects: check that x_ext and y_ext are strictly monotonically increasing (e.g., using np.diff > 0) and raise a clear error or assertion if not, so the creation of u_spline, v_spline, p_spline and subsequent interpolation of ext_sol onto (x_cfd, y_cfd) fails fast with a descriptive message; reference x_ext, y_ext, RectBivariateSpline, u_spline/v_spline/p_spline and ext_sol when adding the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@validation/compare_cavity_external.py`:
- Around line 108-129: The interpolated external fields (ext_sol["u"],
ext_sol["v"], ext_sol["p"]) are reshaped to the CFD grid but plotting in
plot_comparison still uses ext_solver.X/Y and ext_solver.x/y causing
shape/length mismatches; mark when interpolation happens (e.g., set
ext_sol["_interpolated"]=True and store ext_sol["_x"]=x_cfd and
ext_sol["_y"]=y_cfd) and update plot_comparison to, when
ext_sol.get("_interpolated") is True, use ext_sol["_x"] and ext_sol["_y"] (or
cfd_python_result["x"]/["y"]) for contourf(U_mag_ext) and for plotting
centerlines (use ext_sol["u_centerline"] vs ext_sol["_y"] and
ext_sol["v_centerline"] vs ext_sol["_x"]), otherwise fall back to ext_solver.X/Y
and ext_solver.x/y or skip plotting external fields if coordinates are
incompatible.
---
Nitpick comments:
In `@validation/compare_cavity_external.py`:
- Around line 148-154: The external vortex center is left as the original-grid
value (ext_sol["vortex_center"]) even when ext_sol was interpolated; detect
interpolation via ext_sol.get("_interpolated") and if True recompute the vortex
from the interpolated pressure field by finding ext_vortex_idx =
np.unravel_index(np.argmin(ext_sol["p"]), ext_sol["p"].shape) and setting
external_vortex = (x_cfd[ext_vortex_idx[1]], y_cfd[ext_vortex_idx[0]]);
otherwise keep ext_sol["vortex_center"]. Update the comparison block that
currently uses cfd_python_result, cfd_python_vortex and external_vortex to use
this recomputed value when appropriate.
- Around line 115-121: Add input validation before creating the
RectBivariateSpline objects: check that x_ext and y_ext are strictly
monotonically increasing (e.g., using np.diff > 0) and raise a clear error or
assertion if not, so the creation of u_spline, v_spline, p_spline and subsequent
interpolation of ext_sol onto (x_cfd, y_cfd) fails fast with a descriptive
message; reference x_ext, y_ext, RectBivariateSpline, u_spline/v_spline/p_spline
and ext_sol when adding the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65cb8e77-34de-4424-9dd8-56821c31e70c
📒 Files selected for processing (2)
.gitignorevalidation/compare_cavity_external.py
|
|
||
| # Interpolate external result onto cfd_python grid | ||
| x_ext = ext_solver.x | ||
| y_ext = ext_solver.y | ||
| x_cfd = cfd_python_result["x"] | ||
| y_cfd = cfd_python_result["y"] | ||
|
|
||
| u_spline = RectBivariateSpline(y_ext, x_ext, ext_sol["u"]) | ||
| v_spline = RectBivariateSpline(y_ext, x_ext, ext_sol["v"]) | ||
| p_spline = RectBivariateSpline(y_ext, x_ext, ext_sol["p"]) | ||
|
|
||
| ext_sol["u"] = u_spline(y_cfd, x_cfd) | ||
| ext_sol["v"] = v_spline(y_cfd, x_cfd) | ||
| ext_sol["p"] = p_spline(y_cfd, x_cfd) | ||
|
|
||
| # Update centerlines | ||
| mid_x_idx = len(x_cfd) // 2 | ||
| mid_y_idx = len(y_cfd) // 2 | ||
| ext_sol["u_centerline"] = ext_sol["u"][:, mid_x_idx] | ||
| ext_sol["v_centerline"] = ext_sol["v"][mid_y_idx, :] | ||
|
|
||
| print("INFO: Interpolated external result onto cfd_python grid using RectBivariateSpline") |
There was a problem hiding this comment.
Shape mismatch bug: plot_comparison will fail when grids differ.
After interpolation, ext_sol["u"], ext_sol["v"], ext_sol["p"] are reshaped to match cfd_python grid dimensions. However, plot_comparison (lines 247-263, 278, 291) still uses ext_solver.X, ext_solver.Y, ext_solver.x, ext_solver.y for plotting these interpolated arrays:
- Line 247:
contourf(ext_solver.X, ext_solver.Y, U_mag_ext)— shape mismatch - Line 278:
plot(ext_sol["u_centerline"], ext_solver.y)— length mismatch - Line 291:
plot(ext_solver.x, ext_sol["v_centerline"])— length mismatch
This will raise a runtime error when the grid sizes differ.
🐛 Proposed fix: pass interpolated coordinates or skip external field plots when interpolated
One approach is to track whether interpolation occurred and use the appropriate coordinates:
if cfd_python_result["u"].shape != ext_sol["u"].shape:
print(f"WARN: Grid size mismatch: cfd_python {cfd_python_result['u'].shape} vs external {ext_sol['u'].shape}")
# Interpolate external result onto cfd_python grid
x_ext = ext_solver.x
y_ext = ext_solver.y
x_cfd = cfd_python_result["x"]
y_cfd = cfd_python_result["y"]
u_spline = RectBivariateSpline(y_ext, x_ext, ext_sol["u"])
v_spline = RectBivariateSpline(y_ext, x_ext, ext_sol["v"])
p_spline = RectBivariateSpline(y_ext, x_ext, ext_sol["p"])
ext_sol["u"] = u_spline(y_cfd, x_cfd)
ext_sol["v"] = v_spline(y_cfd, x_cfd)
ext_sol["p"] = p_spline(y_cfd, x_cfd)
# Update centerlines
mid_x_idx = len(x_cfd) // 2
mid_y_idx = len(y_cfd) // 2
ext_sol["u_centerline"] = ext_sol["u"][:, mid_x_idx]
ext_sol["v_centerline"] = ext_sol["v"][mid_y_idx, :]
+
+ # Store interpolated grid coordinates for plotting
+ ext_sol["_interpolated"] = True
+ ext_sol["_x"] = x_cfd
+ ext_sol["_y"] = y_cfd
+ else:
+ ext_sol["_interpolated"] = False
print("INFO: Interpolated external result onto cfd_python grid using RectBivariateSpline")Then update plot_comparison to use these coordinates when ext_sol.get("_interpolated") is True.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@validation/compare_cavity_external.py` around lines 108 - 129, The
interpolated external fields (ext_sol["u"], ext_sol["v"], ext_sol["p"]) are
reshaped to the CFD grid but plotting in plot_comparison still uses
ext_solver.X/Y and ext_solver.x/y causing shape/length mismatches; mark when
interpolation happens (e.g., set ext_sol["_interpolated"]=True and store
ext_sol["_x"]=x_cfd and ext_sol["_y"]=y_cfd) and update plot_comparison to, when
ext_sol.get("_interpolated") is True, use ext_sol["_x"] and ext_sol["_y"] (or
cfd_python_result["x"]/["y"]) for contourf(U_mag_ext) and for plotting
centerlines (use ext_sol["u_centerline"] vs ext_sol["_y"] and
ext_sol["v_centerline"] vs ext_sol["_x"]), otherwise fall back to ext_solver.X/Y
and ext_solver.x/y or skip plotting external fields if coordinates are
incompatible.
1 participant
This commit addresses the grid size mismatch warning in
validation/compare_cavity_external.pyby implementing interpolation logic usingscipy.interpolate.RectBivariateSplineto properly compare the CFD-RS output and the external reference array, along with updating their centerlines appropriately.PR created automatically by Jules for task 8917404243692961663 started by @ryancinsight
High-level PR Summary
This PR resolves a TODO in the cavity flow validation script by implementing interpolation logic to handle grid size mismatches between CFD-RS output and external reference data. Instead of returning
Nonewhen grids don't match, the code now usesRectBivariateSplinefrom scipy to interpolate the external solution onto the CFD grid and updates the centerline values accordingly. Additionally,.venv/andvenv/entries are added to.gitignoreto exclude virtual environment directories.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
.gitignorevalidation/compare_cavity_external.pySummary by CodeRabbit
Bug Fixes
Chores